Skip to content

Fix for #7149 + proposed readme for docker on Windows + fixing typos#7150

Merged
ssheorey merged 7 commits intoisl-org:mainfrom
joseph-sch:joseph/various_minor
Apr 2, 2025
Merged

Fix for #7149 + proposed readme for docker on Windows + fixing typos#7150
ssheorey merged 7 commits intoisl-org:mainfrom
joseph-sch:joseph/various_minor

Conversation

@joseph-sch
Copy link
Contributor

@joseph-sch joseph-sch commented Jan 27, 2025 •
edited
Loading

Type

Motivation and Context

  1. There is a typo in o3d.geometry.TriangleMesh.create_mobius and in o3d.t.geometry.TriangleMesh.create_mobius: raidus instead of radius as explained in Typo in argument of o3d.geometry.TriangleMesh.create_mobius and in o3d.t.geometry.TriangleMesh.create_mobius #7149
  2. Some typos throughout the docstrings, comments and a string
  3. Proposing a markdown readme for docker on Windows - or should it be part of the docker.in.rst file in order to appear within https://www.open3d.org/docs/latest/docker.html#building-open3d-in-docker?

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results       (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

@update-docs
Copy link

update-docs bot commented Jan 27, 2025

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@joseph-sch joseph-sch changed the title Proposed minor improvements: fix arg name in create_mobius, fix typos, add a readme for docker on Windows Fix for #7149 + proposed readme for docker on Windows + fixing typos Jan 27, 2025
@joseph-sch joseph-sch marked this pull request as ready for review January 27, 2025 11:48
timohl
timohl approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@timohl timohl left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good small fixes.

I would mark this PR as breaking change though, since a parameter name changed (with good reason).
It is just a typo fix, but existing calls using named parameters would break.

I cannot say much to the content of the docker windows readme, but there is also docker/README.md where this could be included as a section (or in docs/docker.in.rst as you mentioned.
Not sure if it is a good idea to have 3 different spots documenting docker builds.

@joseph-sch
Copy link
Contributor Author

joseph-sch commented Jan 27, 2025

All good small fixes.

I would mark this PR as breaking change though, since a parameter name changed (with good reason). It is just a typo fix, but existing calls using named parameters would break.

I cannot say much to the content of the docker windows readme, but there is also docker/README.md where this could be included as a section (or in docs/docker.in.rst as you mentioned. Not sure if it is a good idea to have 3 different spots documenting docker builds.

Thank you for the feedback

  1. indeed technically it's a breaking change, I've updated the description
  2. I can split the instructions that I proposed between a short mention in docs/docker.in.rst and detailed instructions in docker/README.md - would that be OK?

@timohl
Copy link
Contributor

timohl commented Jan 27, 2025

2. I can split the instructions that I proposed between a short mention in docs/docker.in.rst and detailed instructions in docker/README.md - would that be OK?

Something like that sounds good I think, but somebody else should review that, since I am not using open3d+docker.

ssheorey
ssheorey requested changes Mar 8, 2025
View reviewed changes
Copy link
Member

@ssheorey ssheorey left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @joseph-sch , this is very helpful. Thanks! I can't believe we didn't catch that typo before! Can I recommend merging the WSL docker documentation to the current docker docs in docker/README.md. It's easier for users if it's in a single file.

@joseph-sch
Copy link
Contributor Author

joseph-sch commented Mar 8, 2025

Hi @joseph-sch , this is very helpful. Thanks! I can't believe we didn't catch that typo before! Can I recommend merging the WSL docker documentation to the current docker docs in docker/README.md. It's easier for users if it's in a single file.

Yes indeed - I’m aware that I have to do that but I didn’t get around to doing it yet. I’ll try to do it in the coming 48 hours.

@joseph-sch
Apply requested changes to docs
97e107d 
- Remove docker/BUILD_DOCKER_WSL.md
- Update docker/README.md
- Update docs/docker.in.rst
@joseph-sch
Copy link
Contributor Author

joseph-sch commented Mar 10, 2025

I have removed the extra .md file (BUILD_DOCKER_WSL.md) and incorporated the contents as clarifications in docker.in.rst and in the docker/README.md file. I hope it's fine but of course a review would be good.

Besides, I see there is a conflict to be resolved in cpp/pybind/t/geometry/trianglemesh.cpp.

@ssheorey ssheorey merged commit e2d2bd0 into isl-org:main Apr 2, 2025
18 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ssheorey ssheorey ssheorey approved these changes

+1 more reviewer

@timohl timohl timohl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Typo in argument of o3d.geometry.TriangleMesh.create_mobius and in o3d.t.geometry.TriangleMesh.create_mobius

3 participants

@joseph-sch @timohl @ssheorey